Introduce WellKnownTextConverter interface #2839

greg0ire · 2017-09-03T08:10:24Z

It removes the need for canRequireSqlConversion.
If you are ok with this, I will write a follow up PR on the ORM that will:

trigger a deprecation if any of the canRequireSQLConversion, convertToDatabaseValueSQL, convertToPHPValueSQL. That would probably mean using reflection https://stackoverflow.com/a/17663347/353612
ignore said methods if the type implements the new API

greg0ire · 2017-09-03T08:11:46Z

lib/Doctrine/DBAL/Types/WellKnownTextConverter.php

+ * @author Benjamin Eberlei <kontakt@beberlei.de>
+ * @since  2.0
+ */
+interface WellKnownTextConverter


At first, I named that RequiresSqlConversion, tell me what you prefer, or if you'd like something else. After reading this, I assumed conversion would always be to and from well know text, maybe it's wrong?

I don't really like this name but don't have better proposal ATM, let's think about it...

Yeah me neither :P

Where does "WellKnown" come from? I don't understand that interface name.

If all types extend the base Type class, and it has default implementations for the conversion methods, why do we need an additional interface? The caller doesn't need to know if any conversion is happening within the type implementation, it can just call convert() and let the type take care of the conversion.

I'm not an ORM user, so could you explain briefly what problem is being solved?

@SenseException Ctrl+F "well-know" in the link I gave

@morozov this PR is pedantic; it does not solve any issue, it's just about architectural beauty: if you have a boolean method + methods that should be taken into account only if the boolean method returns true, it just begs to be rewritten with an interface. Note that this will make the Type class less long with 3 methods that don't do anything removed.

But in any case, the caller should either call a method on the object to see if it implements the WKT representation, or check if it implements the interface? The second is even worse because it has to know about two interfaces (Type and WKT) instead of one (Type).

Is it possible to avoid having the caller call this "marker" method at all and just expect it call the convert() method instead?

I don't see beauty in making the Type interface smaller because there's still type-related logic which is implemented on the caller side.

@morozov Please have a look at #2841, the motivation behind this should be clearer.
Basically it's just about killing Type god object, splitting it into smaller pieces and also making consuming Type classes more lightweight (99% of types don't even need this typeof conversion at all, so why would they need to have dummy methods).

greg0ire · 2017-09-03T08:13:17Z

lib/Doctrine/DBAL/Types/WellKnownTextConverter.php

+     *
+     * @return string
+     */
+    public function convertFromWellKnownTextSQL(string $sqlExpr, AbstractPlatform $platform): string;


I deliberately picked other method names than in the old API, so that I could add type hinting.

Actually, since this is targeting 2.7 which will probably require PHP 7.2 (according to current policy of releasing for latest stable PHP), you could add parameter typehints without breaking public API. Not return type though, those could only be done later in 3.0.

I see the RC2 is released, so maybe the final release will come quite soon

ETA @ end of november.

Spaces between return types. The return types will stay in the interface, right? :-)

Not return type though, those could only be done later in 3.0.

@SenseException not completely sure what @Majkl578 meant with that.

Those methods are new, so my guess is, that type widening isn't relevant here and the interface's API with the return type can stay as it is. Return types aren't covered in type widening and he just mentioned that.

The return types will stay in the interface

Not in 2.x, adding return types to ancestors is a BC break, only parameter types are acceptable (and even those only as of PHP 7.2).

As I outlined on Slack, it may be easiest to just not add types at all now, just introduce the interface and add types later when master requires PHP 7.2+.

edit

Those methods are new

Of course, fine for new method names, my point was about original ones, sticking with without convertToDatabaseValueSQL without introducing new renamed methods.

greg0ire · 2017-09-03T09:56:11Z

lib/Doctrine/DBAL/Types/Type.php

@@ -305,6 +305,7 @@ public function canRequireSQLConversion()
     * @param \Doctrine\DBAL\Platforms\AbstractPlatform $platform
     *
     * @return string
+     * @deprecated implement WellKnownTextConverter::convertFromWellKnownTextSQL instead


Should be removed in doctrine 3, along with the 2 other functions

Not implementing the new interface though, which is optional and is not needed for any of the native types.

Right, but existing types that do this type of conversion should implement it, if there are any (maybe not).

I did not find any in the codebase

greg0ire · 2017-09-19T06:31:34Z

ping

Majkl578 · 2017-09-20T03:00:11Z

lib/Doctrine/DBAL/Types/WellKnownTextConverter.php

+     *
+     * @return string
+     */
+    public function convertFromWellKnownTextSQL(string $sqlExpr, AbstractPlatform $platform): string;


Actually, since this is targeting 2.7 which will probably require PHP 7.2 (according to current policy of releasing for latest stable PHP), you could add parameter typehints without breaking public API. Not return type though, those could only be done later in 3.0.

Majkl578 · 2017-09-20T03:04:21Z

lib/Doctrine/DBAL/Types/WellKnownTextConverter.php

+ * @author Benjamin Eberlei <kontakt@beberlei.de>
+ * @since  2.0
+ */
+interface WellKnownTextConverter


I don't really like this name but don't have better proposal ATM, let's think about it...

Majkl578 · 2017-09-20T03:04:51Z

lib/Doctrine/DBAL/Types/WellKnownTextConverter.php

+ *
+ * @author Roman Borschel <roman@code-factory.org>
+ * @author Benjamin Eberlei <kontakt@beberlei.de>
+ * @since  2.0


@since and @author should be updated.

I think we can trust git for this kind of things.

Majkl578 · 2017-09-20T03:05:55Z

lib/Doctrine/DBAL/Types/Type.php

@@ -305,6 +305,7 @@ public function canRequireSQLConversion()
     * @param \Doctrine\DBAL\Platforms\AbstractPlatform $platform
     *
     * @return string
+     * @deprecated implement WellKnownTextConverter::convertFromWellKnownTextSQL instead


Right, but existing types that do this type of conversion should implement it, if there are any (maybe not).

SenseException · 2017-09-20T20:46:24Z

lib/Doctrine/DBAL/Types/WellKnownTextConverter.php

+     *
+     * @return string
+     */
+    public function convertToWellKnownTextSQL(string $sqlExpr, AbstractPlatform $platform): string;


Spaces between return types.

greg0ire · 2017-10-19T13:03:51Z

Rebased

Ocramius · 2017-10-19T13:06:34Z

docs/en/reference/types.rst


+<<<<<<< HEAD


@greg0ire rebase introduced issues here

It removes the need for canRequireSqlConversion.

greg0ire · 2017-11-23T16:20:57Z

I don't really like this name but don't have better proposal ATM, let's think about it...

Does anyone have a better idea?

morozov · 2017-11-23T16:41:41Z

Does anyone have a better idea?

Whatever the name is, I still don't like the idea of having a separate interface which the caller should check against.

It removes the need for canRequireSqlConversion.

But introduces the need for checking implements WellKnownTextConverter which is exactly the same.

Currently, types only can convert values but cannot build SQL which is needed for some of them. Therefore, instead of just converting values, types should be responsible for building SQL overall:

interface Type
{
    function buildQuery(Query Builder $builder, $value);
}

class Integer implements Type
{
    function buildQuery(Query Builder $builder, $value)
    {
        return $builder->createPositionalParameter($value, PDO::PARAM_INT);
    }
}

class Point implements Type
{
    function buildQuery(Query Builder $builder, $value)
    {
        return sprintf(
            'Point(%s, %s)', 
            $builder->createPositionalParameter($value->x),
            $builder->createPositionalParameter($value->y)
        );
    }
}

With this approach, the consumer of the interface doesn't have to make any assumptions, it just delegates the logic of building type-specific query fragments to the type. Always.

greg0ire · 2017-11-23T17:25:57Z

Conversion is not always needed, it's an optional feature, which is exactly what interfaces are for.
With that marker interface, you can get rid of 3 default methods in your object, and from one in your class if you don't need it. If you do need it, you just read the doc, add the implements, and then php will tell you exactly what you need to implement. IDEs will generate a correct skeleton and import use statements for you. So no, it's not exactly the same.

If I understand well, you propose to get rid of the existing check altogether? I don't know the implications, but that sound like a bigger BC-break...

morozov · 2017-11-23T18:01:54Z

Conversion is not always needed, it's an optional feature, which is exactly what interfaces are for.

O-kay…

With that marker interface, you can get rid of 3 default methods in your object, and from one in your class if you don't need it.

It's not about whether somebody needs it or not, it's about what features constitute a type. If it turns out that depending on the type, SQL may be different, then it's up to the type to decide what exactly that SQL should look like (even if there's no additional SQL in most cases), not to the caller — whether it needs/wants to call SQL-specific methods on the type.

Following your logic, the Type::getDefaultLength() only makes string for the String type. Why not introduce a HasLength interface and and only cal $type->getDefaultLength() if it implements HasLength?

If I understand well, you propose to get rid of the existing check altogether? I don't know the implications, but that sounds like a bigger BC-break...

Yes, I'm saying that the calling code shouldn't do any introspection (checking) on the type. I propose, as long as we're doing an API cleanup, introduce a cleaner and more consistent API. Otherwise, it's a replacement for one workaround with another. Yes, a marker interface is a bit better than checking if a method exists, but it's still the same from the caller's standpoint — it still needs to perform a check. The BC concern is secondary. If the new API is better for developers, they will be glad to migrate.

greg0ire · 2017-11-23T20:04:12Z

Following your logic, the Type::getDefaultLength() only makes string for the String type. Why not introduce a HasLength interface and and only cal $type->getDefaultLength() if it implements HasLength?

Well, probably with better naming, but yeah, pretty much. At the moment it's not needed because as you said, only the String type has a length, so Type::getDefaultLength() could probably be removed, just like this comment seems to suggest:

dbal/lib/Doctrine/DBAL/Types/Type.php

Line 144 in 954ce2e

* @todo Needed?

I find that cleaner than calling X methods that do nothing 90% of the time, and maybe you do too, but if seems you have other ideas in mind, like moving more logic from the caller to the type itself? That does sound like a good plan 🤔 especially if it allows to expose less methods publicly. Closing then.

greg0ire commented Sep 3, 2017

View reviewed changes

greg0ire mentioned this pull request Sep 3, 2017

Proofread types doc #2840

Merged

greg0ire commented Sep 3, 2017

View reviewed changes

greg0ire force-pushed the well_know_text_converter branch from 1318862 to 6f72905 Compare September 3, 2017 09:57

greg0ire mentioned this pull request Sep 3, 2017

Refactor type system #2841

Open

9 tasks

greg0ire force-pushed the well_know_text_converter branch from 6f72905 to c615861 Compare September 10, 2017 22:07

Majkl578 reviewed Sep 20, 2017

View reviewed changes

Majkl578 added Improvement New Feature Type Conversion Types labels Sep 20, 2017

SenseException reviewed Sep 20, 2017

View reviewed changes

greg0ire force-pushed the well_know_text_converter branch from c615861 to 5213ef6 Compare October 19, 2017 13:03

Ocramius reviewed Oct 19, 2017

View reviewed changes

greg0ire force-pushed the well_know_text_converter branch from 5213ef6 to 4cc6d30 Compare October 19, 2017 13:14

Introduce WellKnownTextConverter interface

ba55938

It removes the need for canRequireSqlConversion.

greg0ire force-pushed the well_know_text_converter branch from 4cc6d30 to ba55938 Compare November 2, 2017 22:22

greg0ire closed this Nov 23, 2017

greg0ire deleted the well_know_text_converter branch November 23, 2017 20:04

morozov assigned greg0ire Jun 7, 2019

greg0ire mentioned this pull request Apr 14, 2020

Detect index rename/change columns for primary keys as remove/create #3936

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2022


		<<<<<<< HEAD

Introduce WellKnownTextConverter interface #2839

Introduce WellKnownTextConverter interface #2839

Conversation

greg0ire commented Sep 3, 2017

greg0ire Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Majkl578 Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

greg0ire Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Nov 23, 2017

morozov commented Nov 23, 2017 • edited Loading

greg0ire commented Nov 23, 2017

morozov commented Nov 23, 2017 • edited Loading

greg0ire commented Nov 23, 2017 • edited Loading

greg0ire Sep 3, 2017 •

edited

Loading

greg0ire Sep 3, 2017 •

edited

Loading

greg0ire Sep 20, 2017 •

edited

Loading

Majkl578 Sep 20, 2017 •

edited

Loading

greg0ire Sep 3, 2017 •

edited

Loading

morozov commented Nov 23, 2017 •

edited

Loading

morozov commented Nov 23, 2017 •

edited

Loading

greg0ire commented Nov 23, 2017 •

edited

Loading